feat: add warning nudges for cross-region event triggers#10408
feat: add warning nudges for cross-region event triggers#10408shettyvarun268 wants to merge 7 commits intonextfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a warning to alert users when a function in us-central1 is triggered by a resource in a different region, helping to avoid unnecessary cross-region network hops. The review feedback suggests refining the logic to exclude the us multi-region to prevent false positives and improving test isolation by moving cache clearing to the beforeEach block in the test suite.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a warning for potential transatlantic latency hops when functions are deployed in us-central1 but triggered by events in non-US regions. It includes logic to aggregate these warnings and provides an environment variable to suppress them. Comprehensive unit tests have been added to verify the warning behavior and suppression flag. Feedback was provided to improve the efficiency of the warning logic by moving the environment variable check outside the loop and to reduce code nesting in accordance with the repository style guide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a warning in triggerRegionHelper.ts for functions in us-central1 with triggers in non-US regions to prevent latency issues, along with an environment variable for suppression and new unit tests. Feedback recommends improving the environment variable check to explicitly compare against the string "true" and filtering out gcfv1 functions for consistency.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a warning mechanism in ensureTriggerRegions to alert users when GCFv2 event-triggered functions are deployed in us-central1 while their triggers are located in non-US regions, helping to avoid unnecessary cross-region latency. It includes an environment variable FIREBASE_SUPPRESS_REGION_WARNING to disable these alerts and adds comprehensive unit tests to verify the warning logic, including rolled-up messages for multiple functions. I have no feedback to provide.
|
|
||
| const offendingFunctions: string[] = []; | ||
| for (const ep of backend.allEndpoints(want)) { | ||
| if (ep.platform === "gcfv1" || !backend.isEventTriggered(ep)) { |
There was a problem hiding this comment.
Why are we ignoring v1 functions? Is there a reason those should stay with international hops?
There was a problem hiding this comment.
I am still debating this. One one hand I agree that the v1 users might get the biggest benefit out of this since they might have deployed in different regions. But at the same time v1 functions would require some additional lookup which would make the deploy process slower for them(especially with multiple functions).
There was a problem hiding this comment.
Can you quantify this at all? How complicated and slow is the additional lookup? If we're increasing the deployment time from like 1 minute to 1:02 ... I'm fine with that slowdown. If we were going from 0:30 to 1:30 that would be a problem.
There was a problem hiding this comment.
For small projects with a few functions, we could probably parallelize the lookups keeping the delay under 1 to 2 seconds (so within your limit!).
The problem really comes down to scale and permissions. If a user has 20+ V1 functions pointing at several distinct databases or buckets, making individual API hits to solve those missing region fields on V1 manifests could tack on 10+ seconds of dead weight to the deploy time. Also, doing high-speed sequential hits runs a risk of tagging rate limits or firing IAM check fails for developers on locked down corporate accounts.
| "functions", | ||
| `The following functions have triggers in different regions than they are located:\n` + | ||
| `- wantFn (us-central1, Trigger: europe-west1)\n` + | ||
| `To avoid unnecessary cross-region network hops, you should explicitly assign these functions to their trigger regions. ` + |
There was a problem hiding this comment.
you should explicitly assign these functions to their trigger regions reads as if the user hasn't assigned a region, but this warning also fires when they've explicitly chosen us-central1 right?
Maybe something like "Consider assigning these functions to their trigger regions / consider collocating"
There was a problem hiding this comment.
or we distinguish default vs. explicit assignment upstream and only recommend reassignment in the default case?
There was a problem hiding this comment.
Good catch on the copy. I will update the message to 'Consider assigning these functions to their trigger regions or collocating them' to handle cases where the user explicitly chose us-central1. Regarding distinguishing default vs. explicit assignment: it turns out the SDK outputs us-central1 in both cases to the manifest, so the CLI cannot distinguish them at this stage. Updating the copy is the safest fix for now.
shettyvarun268
force-pushed
the
clean-event-warning
branch
from
05a635d to
d506809
Compare
4 participants
This PR implements robust cross-region latency warnings within the Firebase Functions deployment pipeline to nudge developers away from unnecessary transoceanic network hops. When an event-triggered function (such as Firestore or Cloud Storage) is explicitly assigned or defaults to us-central1 while its backing event resource operates in a distinct non-US location, the CLI now issues a user-friendly cautionary warning encouraging explicit collocation with the event trigger's region.
To maintain a clean CLI experience, the PR aggregates multiple offending functions into a single, consolidated warning log to avoid spamming the terminal during large deployments. It correctly exempts US multi-region databases (such as nam5 and nam7) to prevent false-positives, and introduces an opt-out environment flag (FIREBASE_SUPPRESS_REGION_WARNING=true). The flag allows headless CI/CD pipelines to silence repetitive warnings safely, with instructions on how to apply it printed directly inside the warning log.